Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make A-hjkl tree-sitter nav A-poin #2205

Merged
merged 1 commit into from
Apr 29, 2022
Merged

Make A-hjkl tree-sitter nav A-poin #2205

merged 1 commit into from
Apr 29, 2022

Conversation

pickfire
Copy link
Contributor

@pickfire pickfire commented Apr 20, 2022 •
edited
Loading

A-hl currently is not very consistent with hl when next object is
selected, since it may go up/down or left/right and this behavior is
confusing such that some people think it should swap the keys with A-jk,
so it is better to use A-pn since that only specifies two direction.

A-jk have the same issue as in it usually moves right and is not
consistent with the behavior of jk so people may think A-hl is better,
maybe A-oi is better here since A-hl will be swapped to A-pn, A-oi can
convey the meaning of in and out, similar to some window manager keys?

cc @the-mikedavis since he came up with the original keys, but this was discussed with him in the chat

Note sure what to do with the existing A-left, A-right, A-up, A-down so I just leave it

Fix #2202

@the-mikedavis
Copy link
Member

I was curious which should be shrink vs. expand with i and o. I suppose i could be into the tree (shrink) and o out of the tree (expand), but OTOH i jumps forward in the jumplist and o backwards which might map better to i expand, o shrink.

I'm not sure which way makes more sense 🤔

@pickfire
Make A-hjkl tree-sitter nav A-pion
04d8ce5 
A-hl currently is not very consistent with hl when next object is
selected, since it may go up/down or left/right and this behavior is
confusing such that some people think it should swap the keys with A-jk,
so it is better to use A-pn since that only specifies two direction.

A-jk have the same issue as in it usually moves right and is not
consistent with the behavior of jk so people may think A-hl is better,
maybe A-oi is better here since A-hl will be swapped to A-pn, A-oi can
convey the meaning of in and out, similar to some window manager keys?
@pickfire
Copy link
Contributor Author

pickfire commented Apr 20, 2022 •
edited
Loading

I was curious which should be shrink vs. expand with i and o. I suppose i could be into the tree (shrink) and o out of the tree (expand), but OTOH i jumps forward in the jumplist and o backwards which might map better to i expand, o shrink.

Oh, wait I thought it was a typo. But shouldn't A-i go in (shrink) and A-o go out (expand)?

OTOH i jumps forward in the jumplist and o backwards which might map better to i expand, o shrink

I don't quite get that analogy there, that is forward and backward but I see very little relationship between forward with expand and backward with shrink, compared to in/into/inside (I use but you used into) with shrink and out/outside with expand. Or maybe I should just rename it to in and out to let other people understand the analogy easier?

@the-mikedavis
Copy link
Member

Oh actually yeah, let's do i for shrink and o for expand. I think that makes sense intuitively

@pickfire
Copy link
Contributor Author

pickfire commented Apr 21, 2022 •
edited
Loading

Should I go change the docs to make it in/into/inside and out/outside to make it more intuitive? I also don't know which word to use among those.

@the-mikedavis
Copy link
Member

Nah I think the docs are fine as is. IMO mnemonics don't really belong there and "out" and "in" don't really make sense themselves.

@the-mikedavis
Copy link
Member

I say :shipit: for these bindings but I'm curious what others have to say

@sudormrfbin
Copy link
Member

Oh actually yeah, let's do i for shrink and o for expand. I think that makes sense intuitively

Agree with this, feels intuitive considering the default keybinds of ctrl-{i,o} for the jumplist. Alt-{n,p} sounds like a good change too 👍🏾.

@tomKPZ
Copy link
Contributor

tomKPZ commented Apr 24, 2022

This change looks good to me. I want to add a "select all children" binding (in addition to the current "select first child"), which doesn't fit into the HJKL model anyway.

@altsem
Copy link

altsem commented Apr 24, 2022

I'd love to use these bindings on a non-us layout Mac. It's a complete mess with binding the alt-key on a Mac with a Swedish keyboard layout. I digged the SpaceVim bindings for this: just keep pressing v (V to shrink?).

@archseer
Copy link
Member

I'd love to use these bindings on a non-us layout Mac. It's a complete mess with binding the alt-key on a Mac with a Swedish keyboard layout. I digged the SpaceVim bindings for this: just keep pressing v (V to shrink?).

Hmm, interesting. we could have some kind of submode you'd enter that would have these types of sticky mappings. For now though, you can remap these alt keys in your user config

@archseer archseer merged commit ab6a00e into helix-editor:master Apr 29, 2022
@helmesjo
Copy link

helmesjo commented Jun 2, 2022 •
edited
Loading

Slight bug noticed (most likely not specifically related to this PR):

  1. Open a file.
  2. Keep hitting A-o until whole file selected.
  3. At this moment, as many times as you keep hitting A-o (everything is already selected, so nothing happens visually), the same amount of times you need to hit A-i to "undo" it before you start seeing things deselect (it basically buffers up, even though there is nothing else to select).

@the-mikedavis
Copy link
Member

Could you create a separate issue for that? Or if you'd like to work on a PR directly, I think this could be solved by checking the head of view.object_selections to see if we're pushing the same selection

helix/helix-term/src/commands.rs

Lines 3776 to 3777 in 4f3d0a7

// save current selection so it can be restored using shrink_selection
view.object_selections.push(current_selection.clone());

@pickfire pickfire deleted the consistent-treesitter-nav branch June 2, 2022 23:19
@helmesjo
Copy link

helmesjo commented Jun 8, 2022

@the-mikedavis Sorry for late reply. Added issue #2714.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Swap alt-j with alt-l and alt-k with alt-h
7 participants
@pickfire @the-mikedavis @sudormrfbin @tomKPZ @altsem @archseer @helmesjo